Skip to content

Simplify server setup#43

Merged
joachimvh merged 40 commits intoproject/pacsoi-poc1from
feat/simplify
Apr 24, 2025
Merged

Simplify server setup#43
joachimvh merged 40 commits intoproject/pacsoi-poc1from
feat/simplify

Conversation

@joachimvh
Copy link
Copy Markdown
Contributor

Probably going to keep this branch open for a bit until it feels like a finished whole.

Feel free to comment on commits that might be doing something wrong.

@termontwouter
Copy link
Copy Markdown
Collaborator

Hi Joachim, thanks for working on this. 💪

Just a few thoughts at this point of the process. Overall, I agree with most of the changes, and, on the few points that I raised an eyebrow, I'm mostly happy to ignore our different approach and let you take care of those implementations decisions.

My main remark is that a number of changes fail to live up to the title of this PR ☝️ They bring the code more in line with the CSS codebase, but do so at the cost of some of its initial simplicity. For example, one commit simplifies the use of input arguments from context.request.* to request.*, but a later one changes this to input.request.* and input.operation.*; in other places, simple objects with native Node.js HTTP fields are swapped for wrappers that are not strictly necessary (at least for now). While I am okay with that in as far as alignment with the CSS is desired, at least on two points this does not seem to bring any added benefit:

  • The first is the 50-line BaseTargetExtractor, which wraps an OriginalUrlExtractor just to change one of its parameters, yet only saves about 5 lines of the original code. If even for those few lines you'd rather use the CSS mechanism, can we not implement the specific IdentifierStrategy alone, and pass it to the OriginalUrlExtractor in config?

  • The second is the routing mechanism. I fail to see how the new InteractionRouterHandler (with a trivial MatchAllRoute to make up for its complexity) is better than the original router handlers. They both basically work as a map; the original code does so very simply, the new code in a roundabout way. Something in the back of my head screams a vague "pre-opture matimization" 🙈

We've definitely reached a milestone though: a Handlers.js-less codebase 😄 🚀

@joachimvh
Copy link
Copy Markdown
Contributor Author

So "Simplify the server setup" might indeed be a bit too ...simple of a title. Other aspects are robustness, future extensibility, etc. But I agree that this PR currently is not perfect, it also has some aspects that I'm not completely happy with yet.

One of the main reasons for trying to solve as many problems as possible with CSS components is to then potentially extract those from the CSS repo itself into separate repos so we, for example, have a standard library for the initial HTTP server setup. That way, there is less chance of running into the same problems multiple times.

The problem is of course that while CSS was designed to be as modular as possible, there are limits, and some things in there are still very specific for the goal it is trying to achieve. E.g., one of the core aspects is to have streaming data, while in the project here this just introduces extra hassle at some aspects. Same thing with the metadata object being used as context, it is very nice and generic, but perhaps might be a bit too much here. Currently I'm considering getting rid of the Operation object completely here and again use a simpler object with just data headers and some other fields. This all to say that I'm also still figuring out what feels good 😅.

BaseTargetExtractor

This class should just not exist. The functionality of the OriginalUrlExtractor from the CSS is very useful to have a separate block for getting the request URL, but unfortunately it also has a small part to check if the input URL falls within the accepted domain. The class here is to work around that, but what I actually want is to split that up in the CSS so that class can be reused.

InteractionRouterHandler

Yea it might be a bit too much, I'm also not convinced yet. Part of it was also reusing the CSS components when possible again, and also some robustness, but it might be too much. The problem was also, similar to the previous one, that the routing classes from CSS were a bit too specific. I feel like this class also shows the issue of using the RDF metadata object for the context.

@termontwouter
Copy link
Copy Markdown
Collaborator

Perfect! I think we're overall very much on the same line then. Good to know that in your mind this is also still WIP, with more or less the same doubts as I expressed. I fully agree on the importance of modularity and reuse, and have always been a proponent of getting more generic stuff out of the CSS in separate packages. Continue the great work! 🚀

@joachimvh joachimvh force-pushed the feat/simplify branch 2 times, most recently from 1b5af5a to 184d536 Compare March 20, 2025 12:47
@joachimvh
Copy link
Copy Markdown
Contributor Author

I have reverted some of the changes I did and redid them, staying closer to the original source, with the idea that this is already sufficient for the intended goal. Otherwise it would lead me too far trying to change too much.

@joachimvh joachimvh force-pushed the feat/simplify branch 2 times, most recently from 68ed5f0 to 28db382 Compare March 24, 2025 12:56
Copy link
Copy Markdown
Collaborator

@termontwouter termontwouter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, mostly agree with the changes since last time I reviewed.

Not sure about the JsonFormHttpHandler

  • Request (content) format and response (accept) format are not necessarily related to each other. Instead of nesting the routes in this single handler, I think it would be better if we split it up in a parsing handler and a serializing handler, and place those in a sequence before and after the routes.
  • Merging JSON logic and URL-encoded form logic into a single class feels a bit unclean. Better make two classes out of it in a waterfall system, no?

Not sure why [the validation of the grant_type parameter] was commented out.
I might accidentally discover why at some point in the future after much frustration.

Because the server was (initially) not meant to handle any other grant type anyway 🤷

@joachimvh
Copy link
Copy Markdown
Contributor Author

Not sure about the JsonFormHttpHandler

It is the behaviour of this class that caused me to revert some changes because I kept trying to make it perfect. 😀

What I wanted, minimally, was that the route classes did not have to worry about data parsing and could assume they get a JSON object as input, and similarly, that they could just return a JSON object.

On the other hand I wanted the outer server layer to just always receive a buffer or similar and not have to convert. The JsonFormHttpHandler is the middleware in between so nothing else has to worry about that.

It is currently hardcoded to JSON and form data, as those are the two relevant formats and they are quite similar, for now at least, with the idea that if it ever needs to do more it can be replaced. I first thought about importing the whole converter block from CSS but that seemed to much for a "simple" API.

It might indeed be a bit nicer to have a middleware class that takes as input a parser and serializer, but to me it didn't really feel necessary to abstract that part right now.

@joachimvh joachimvh changed the base branch from main to project/pacsoi-poc1 April 15, 2025 08:28
@joachimvh
Copy link
Copy Markdown
Contributor Author

joachimvh commented Apr 15, 2025

Rebased changes onto the pacsoi branch. All commits are the same as before except the first one which fixes an issue in the pacsoi branch.

@joachimvh joachimvh marked this pull request as ready for review April 15, 2025 10:59
Copy link
Copy Markdown
Collaborator

@termontwouter termontwouter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very minor nits:

  • Not sure where CORS handling happens now 🤔
  • Not sure what the reasons are for removing certain TSDocs while cleaning up others 🤔

if (!token) throw new Error('Token not found.');

return this.jwtTokenFactory.serialize(Object.assign(token, { active: true }));
return this.jwtTokenFactory.serialize({ ...token, active: true } as AccessToken);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very minor, mostly a question out of interest: why the preference for destructuring, if you need a cast to make it work?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't actually remember doing this, so it is even possible this happened accidentally with all the merging of branches 😀. But I do prefer the second option as the first one modifies the token object, so perhaps that is why I did it.

@joachimvh
Copy link
Copy Markdown
Contributor Author

  • Not sure where CORS handling happens now 🤔

The CSS cors handler is used.

  • Not sure what the reasons are for removing certain TSDocs while cleaning up others 🤔

Quite random mostly 😅. I removed those where I noticed my IDE complained that there was a mismatch between the parameters of the docs and the function, or the comments were just wrong. But I didn't focus too much on it under the assumption that things were still going to change.

@termontwouter
Copy link
Copy Markdown
Collaborator

The CSS cors handler is used.

And that handler is configured elsewhere in the default configs? Because I only noticed the removal of the handlersjs one from the router config, but not an inclusion of the CSS one elsewhere 🤔

For me, this branch and the config one can be merged in the PACSOI one when you feel they're ready. Before merging PACSOI into main, I'd prefer to have a branch in-between, though, to remove all the stuff from PACSOI that we don't want in main.

@joachimvh
Copy link
Copy Markdown
Contributor Author

And that handler is configured elsewhere in the default configs?

It's currently added here:

Before merging PACSOI into main, I'd prefer to have a branch in-between, though, to remove all the stuff from PACSOI that we don't want in main.

Any reason to prefer this to removing stuff afterwards after discovering it's not needed? Because at this point I can't really say what is relevant and what isn't. Except for removing the screencast and updating the readme (the latter probably just back to the previous version).

I'm going to keep the branches separate until merging into main to still keep somewhat of an overview.

@termontwouter
Copy link
Copy Markdown
Collaborator

Any reason to prefer this to removing stuff afterwards after discovering it's not needed? Because at this point I can't really say what is relevant and what isn't. Except for removing the screencast and updating the readme (the latter probably just back to the previous version).

I'd prefer main not to be filled with stuff that's irrelevant for the main branch, but I also understand your perspective. Maybe remove already what you see immediately, and leave the rest for when we bump into it?

@joachimvh joachimvh merged commit 8669be7 into project/pacsoi-poc1 Apr 24, 2025
3 checks passed
@joachimvh joachimvh deleted the feat/simplify branch April 24, 2025 07:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants